According to our previous discussion, I thought we should disable the block import notification until the snap sync is finished so we don't need maybe_cache_block_info here.

AFAIU, it will cause a deadlock - we need consensus chain import working because we snap sync consensus chain to the segment starting block and need to import some blocks before we reach the consensus chain block related to the last confirmed domain block.

Okay, I see. But even so, we don't need to cache and process every block import notification, the underlying domain-block-processor doesn't make assumptions about the notification stream to emit notification for every block, this was used to handle cases like chain re-org and node restart, see:

So we don't need to cache the notification but instead ignore it.

The caching method ignores the block number less than the target block. Did I understand your comment correctly?

The caching method ignores the block number less than the target block.

Actually, ignore all the block import notifications until the domain sync is finished, the above linked code will properly handle all the missed block notifications.

The slot notification will be skipped if the consensus chain is in major sync so this check may be unnecessary

Absence of this check produces this error:

 ERROR Domain: domain_client_operator::domain_worker: Error at producing bundle. slot=Slot(119) err=Backend("Domain block hash for #529 not found")

This (and the relayer error) is probably because the consensus chain sync oracle is used, when consensus chain sync finishes the domain chain sync just starts so the domain block is still missing.

We can use a customized domain sync oracle here and in other places (e.g. the relayer) which:

• When full sync is used, return the result of the consensus chain sync oracle
• When domain snap sync is used, return the result of snap_sync_orchestrator.domain_snap_sync_finished

Not sure I understood your comment. This error most likely arises when we try to access the inaccessible block and it makes sense because we don't have those blocks then. The relayer error seems to be caused by the missing runtime state with a similar reason.

What sync oracle do you have in mind?

Nazar described a similar solution offline mentioning SubspaceSyncOracle

This error most likely arises when we try to access the inaccessible block and it makes sense because we don't have those blocks then.

Yeah, what I propose is to skip processing the slot notification/block import notification/relayer until the domain sync is finished (at that point the required domain block and state should be available). Using snap_sync_orchestrator.domain_snap_sync_finished also work in some places but IMO sync oracle is a better abstraction.

Nazar described a similar solution offline mentioning SubspaceSyncOracle

Maybe the same thing, but we need a DomainSyncOracle which can't be used by the consensus chain.

The relayer will skip the block when the consensus chain is in major sync (we can change it to check if the domain chain is in major sync if that is supported) so this check may be unnecessary.

It produces the following error:
2024-10-14T14:17:25.818009Z ERROR Domain: message::relayer: Failed to submit messages from the chain Domain(DomainId(0)) at the block (815 err=FetchAssignedMessages

Is this documented in the spec somewhere? We have much higher numbers for the consensus chain.

Also it is usually nice to have these at the top of the file for easier discovery.

I moved the constants and increased their values for now, however, it seems we'll change either their values or meaning because of the issue found by Ning in the comments below.

What do you think about making this Option<BlockNumber> instead and decide how to orchestrate block number retrieval externally?

Specifically with other suggestions it'll be simply awaiting on a oneshot channel before starting snap sync, shouldn't need a while provider just for oneshot receiver.

I don't think I follow you here.

The actual sync function works with the target_block: Option<BlockNumber as you suggested, snap_sync wrapper handles the acquisition of the target block. We can move this acquisition up the call stack but I consider that more confusing. Also, the target block provider trait incapsulates the default non-blocking behavior of the consensus chain sync and IMO is better than the one-shot receiver because of it.

Nit: this is an unusually strong ordering, which might have a performance impact. Usually it is enough to use Acquire for loads and Release for stores.

I'll set it to the proposed values. However, I don't think performance suffers here because of the access pattern (write once in total, query once per second). Please, correct me if I'm wrong.

Besides the domain worker, SubspaceSyncOracle is also used in other places of the consensus chain like the archiver, PoT worker and RPC, etc, and if the domain sync is not finished it will change the return value of is_major_syncing and some behaviors of these workers, better to have more eyes on this to see if it is okay.

The SeqCst ordering has an impact on read and write performance, because it imposes a total order across all threads:
https://doc.rust-lang.org/std/sync/atomic/enum.Ordering.html#variant.SeqCst

Similar to my last comment, Release should be enough here:
https://doc.rust-lang.org/std/sync/atomic/enum.Ordering.html

Just un-resolving this because Ning wanted more people to review the code.

I'll set it to the proposed values. However, I don't think performance suffers here because of the access pattern (write once in total, query once per second). Please, correct me if I'm wrong.

Since the imported_block_notification_stream only poll after block_importing_notification_stream reaches the target block, at that point there may be too many items pending inside theimported_block_notification_stream and occupy a lot of memory, it may better if we can poll these 2 streams concurrently like:

let mut is_importing_reach_target = false;
let mut is_imported_reach_target = false;
loop {
    tokio::select! {
        Some(import_notification) = imported_block_notification_stream.next() if !is_imported_reach_target => {
            ...
            is_imported_reach_target = block_number >= target_block_number;
        },
        Some((block_number, mut acknowledgement_sender)) = block_importing_notification_stream.next() if !is_importing_reach_target => {
            ...
            is_importing_reach_target = block_number >= target_block_number;
        },
        else => {
            if is_importing_reach_target && is_imported_reach_target {
                break
            }
        }
    }
}

I expect only a segment of the blocks in the worst case (25-35k). The block importing notification consists mostly of the header and several other fields (in rare cases TreeRoute structure). Let's say it will have the size of 1KB on average, we will occupy 30MB in the memory for 5 min. I prefer the current straightforward code because of its simplicity and readability. Let me know if you think it's a critical point and we'll reconsider the snippet.

Not critical but something can be improved, the snippet is also more succinct IMHO, non-blocker.

The underlying stream type is created here:
https://github.com/autonomys/polkadot-sdk/blob/819a5818284a96a5a5bd65ce67e69bab860d4534/substrate/client/service/src/client/client.rs#L1953

It will warn on 100,000 items, so that seems ok for now, but maybe we could leave a TODO here?

I agree that the suggested snippet is more succinct, however, IMO it's better to separate those streams because the first one impacts other parts of the system and is a source of deadlocks in two networks. It's easier to deal with it this way.

Besides the domain worker, SubspaceSyncOracle is also used in other places of the consensus chain like the archiver, PoT worker and RPC, etc, and if the domain sync is not finished it will change the return value of is_major_syncing and some behaviors of these workers, better to have more eyes on this to see if it is okay.

Sorry I'm unable to see why we need both domain-operator-worker and domain-operator-worker-starter, could you give more detail about this?

start_worker_task waits for the target block from the blocking channel to clean importing streams and without running in a new async task it causes a deadlock.

Not really deadlock, please try to make the whole start_worker_task an async block e.g. let start_worker_task = async move { .. } and removing domain-operator-worker-starter should be fine.

This snippet has two goals: 1) block start_worker method until we get the target block, and 2) unblock the operator code to allow getting the target block from the domain operator network.

The first goal was initially achieved within start_worker using the orchestrator but later was refactored to be controlled from the outside. The second goal was introduced when we moved the target block acquisition from the consensus node network to the domain node network (after the last confirmed domain block execution receipt protocol change).

Yeah, I understand what led to this change, my point is we don't need the domain-operator-worker-starter as a wrapper of domain-operator-worker. IIUC what we want is 3 sequential steps:

1. Get the target_block_number
2. Drain the steams
3. start_worker

This can be done by wrapping them into one async block:

let start_worker_task = async move {
	let target_block_number = target_block_receiver.recv().await;
	
	while let Some(...) = stream.next().await {
		...
	}

	crate::domain_worker::start_worker(...).boxed()
};
spawn_essential.spawn_essential_blocking(
	"domain-operator-worker",
	None,
	Box::pin(start_worker_task),
);

This doesn't work in the previous commit 60d45e9 because the start_worker_task is not an async block (i.e. let start_worker_task = { ... }) and is evaluated as a future of the last stepstart_worker but not a future of all the 3 steps, and first 2 steps will block the whole function Operator::new()

You were correct. It does work. The original solution was developed evolutionarily but it is possible to simplify it indeed. Thanks!

The return value of this future is Result<bool> but it seems like Result<()> would be enough as the bool is not used and dropped right away.

This code has already been changed in the main branch. I'll merge it after the last approval.